Skip to content

Enable opting out of TSVI #1128

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 50 commits into from
Closed

Enable opting out of TSVI #1128

wants to merge 50 commits into from

Conversation

@penelopeysm
Copy link
Member

@penelopeysm penelopeysm commented Nov 8, 2025 •
edited
Loading

Performance gains in #1132 are only really significant when not using TSVI.

TSVI gets used more than it should (#1086).

This PR provides a mechanism to globally opt out of TSVI, in a way that is fully backwards-compatible.

Changing this to a global opt-in would just require changing the default to false.

A per-model opt-in would be doable too, albeit requiring more work. I think it's better to start from a global one and only consider a per-model one if people complain.

Performance

Edit: benchmarking this, I just realised that dereferencing USE_THREADSAFE_EVAL gives rise to more overhead than checking Threads.nthreads(). A per-model one should make this work better because if it's stored in the type, it can be removed at compile time. I'll investigate this further.

@penelopeysm penelopeysm mentioned this pull request Nov 8, 2025
Closed
@github-actions
Copy link
Contributor

github-actions bot commented Nov 8, 2025 •
edited
Loading

Benchmark Report for Commit 6a0ecfa

Computer Information

Julia Version 1.11.7
Commit f2b3dbda30a (2025-09-08 12:10 UTC)
Build Info:
  Official https://julialang.org/ release
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: 4 × AMD EPYC 7763 64-Core Processor
  WORD_SIZE: 64
  LLVM: libLLVM-16.0.6 (ORCJIT, znver3)
Threads: 1 default, 0 interactive, 1 GC (on 4 virtual cores)

Benchmark Results

┌───────────────────────┬───────┬─────────────┬───────────────────┬────────┬────────────────┬─────────────────┐
│                 Model │   Dim │  AD Backend │           VarInfo │ Linked │ t(eval)/t(ref) │ t(grad)/t(eval) │
├───────────────────────┼───────┼─────────────┼───────────────────┼────────┼────────────────┼─────────────────┤
│ Simple assume observe │     1 │ forwarddiff │             typed │  false │            7.5 │             1.7 │
│           Smorgasbord │   201 │ forwarddiff │             typed │  false │          757.0 │            43.8 │
│           Smorgasbord │   201 │ forwarddiff │ simple_namedtuple │   true │          436.1 │            55.2 │
│           Smorgasbord │   201 │ forwarddiff │           untyped │   true │          798.1 │            37.1 │
│           Smorgasbord │   201 │ forwarddiff │       simple_dict │   true │         7229.9 │            27.8 │
│           Smorgasbord │   201 │ forwarddiff │      typed_vector │   true │          776.1 │            42.4 │
│           Smorgasbord │   201 │ forwarddiff │    untyped_vector │   true │          795.6 │            37.9 │
│           Smorgasbord │   201 │ reversediff │             typed │   true │          920.4 │            45.5 │
│           Smorgasbord │   201 │    mooncake │             typed │   true │          734.1 │             6.0 │
│           Smorgasbord │   201 │      enzyme │             typed │   true │          919.1 │             4.0 │
│    Loop univariate 1k │  1000 │    mooncake │             typed │   true │         4035.9 │             5.7 │
│       Multivariate 1k │  1000 │    mooncake │             typed │   true │         1083.7 │             8.6 │
│   Loop univariate 10k │ 10000 │    mooncake │             typed │   true │        44505.8 │             5.4 │
│      Multivariate 10k │ 10000 │    mooncake │             typed │   true │         8929.4 │             9.9 │
│               Dynamic │    10 │    mooncake │             typed │   true │          130.1 │            13.0 │
│              Submodel │     1 │    mooncake │             typed │   true │            9.5 │            11.2 │
│                   LDA │    12 │ reversediff │             typed │   true │         1012.2 │             2.1 │
└───────────────────────┴───────┴─────────────┴───────────────────┴────────┴────────────────┴─────────────────┘

@codecov
Copy link

codecov bot commented Nov 8, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 94.48276% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.69%. Comparing base (4ca9528) to head (6a0ecfa).
⚠️ Report is 3 commits behind head on breaking.

Files with missing lines Patch % Lines
src/onlyaccs.jl 70.00% 3 Missing ⚠️
src/compiler.jl 85.71% 2 Missing ⚠️
ext/DynamicPPLEnzymeCoreExt.jl 0.00% 1 Missing ⚠️
src/fasteval.jl 98.61% 1 Missing ⚠️
src/utils.jl 66.66% 1 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##           breaking    #1128      +/-   ##
============================================
+ Coverage     81.32%   81.69%   +0.36%     
============================================
  Files            40       42       +2     
  Lines          3807     3922     +115     
============================================
+ Hits           3096     3204     +108     
- Misses          711      718       +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

penelopeysm
penelopeysm commented Nov 8, 2025
View reviewed changes
@github-actions
Copy link
Contributor

github-actions bot commented Nov 8, 2025

DynamicPPL.jl documentation for PR #1128 is available at:
https://TuringLang.github.io/DynamicPPL.jl/previews/PR1128/

Base automatically changed from py/fastinit to py/fastldf November 10, 2025 19:28
@penelopeysm
Fast Log Density Function
5bd9748
@penelopeysm
Make it work with AD
38d63f4
@penelopeysm
Optimise performance for identity VarNames
0b475ca
@penelopeysm
Mark get_range_and_linked as having zero derivative
9123b2b
@penelopeysm
Update comment
326f4b1
@penelopeysm
make AD testing / benchmarking use FastLDF
18c2d2e
@penelopeysm
Fix tests
d5f58f8
@penelopeysm
Optimise away make_evaluate_args_and_kwargs
5620efe
@penelopeysm
const func annotation
56a1bbf
@penelopeysm
Disable benchmarks on non-typed-Metadata-VarInfo
e795d7a
@penelopeysm
Fix _evaluate!! correctly to handle submodels
2e7c723
@penelopeysm
Actually fix submodel evaluate
35116b1
@penelopeysm
Document thoroughly and organise code
abe4068
@penelopeysm
Support more VarInfos, make it thread-safe (?)
01ba783
@penelopeysm
fix bug in parsing ranges from metadata/VNV
24a4519
@penelopeysm
Fix get_param_eltype for TSVI
b36ada6
@penelopeysm
Disable Enzyme benchmark
379abfd
@penelopeysm
Don't override _evaluate!!, that breaks ForwardDiff (sometimes)
05d7b95
@penelopeysm
Move FastLDF to experimental for now
3d206f4
@penelopeysm
Fix imports, add tests, etc
007da25
@penelopeysm
More test fixes
1ee190f
@penelopeysm
Concretise parameter vector in tests
7a6d0be
@penelopeysm
Add zero-allocation tests
587fa45
@penelopeysm
Add Chairmarks as test dep
045385e
@penelopeysm
Disable allocations tests on multi-threaded
6e9fc1d
@penelopeysm
Fast InitContext (#1125)
1fee38f 
* Make InitContext work with OnlyAccsVarInfo

* Do not convert NamedTuple to Dict

* remove logging

* Enable InitFromPrior and InitFromUniform too

* Fix `infer_nested_eltype` invocation
@penelopeysm
Refactor FastLDF to use InitContext
40ac87d
@penelopeysm
note init breaking change
83add24
@penelopeysm
fix logjac sign
f840dbc
@penelopeysm
workaround Mooncake segfault
5d54e13
@penelopeysm
fix changelog too
c129125
@penelopeysm
Fix get_param_eltype for context stacks
00ad3ac
@penelopeysm
Add a test for threaded observe
a371b88
@penelopeysm
Export init
2c3563c
@penelopeysm
Remove dead code
f12b407
@penelopeysm
fix transforms for pathological distributions
de88c78
@penelopeysm
Tidy up loads of things
1156a49
@penelopeysm
fix typed_identity spelling
a3a4795
@penelopeysm
fix definition order
cda780a
@penelopeysm
Improve docstrings
99a4a20
@penelopeysm
Remove stray comment
b58f90a
@penelopeysm
export get_param_eltype (unfortunatley)
bf00ca1
@penelopeysm
Add more comment
6e8c146
@penelopeysm
Update comment
62a8746
@penelopeysm penelopeysm mentioned this pull request Nov 12, 2025
Merged
@penelopeysm penelopeysm changed the base branch from py/fastldf to py/ldf November 12, 2025 12:15
Base automatically changed from py/ldf to breaking November 13, 2025 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@penelopeysm penelopeysm

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@penelopeysm